Do not derive Clone/Copy/Eq*/Ord* for opaque types by elichai · Pull Request #1667 · rust-lang/rust-bindgen

elichai · 2019-11-04T16:19:33Z

Hi,
This is an attempt to solve #1656
I'm not sure if the IsOpaque trait is defined only on actually opaque types. but I used that to check and not impl anything except Debug/Default

emilio

Observables look good, but the place to check it is not quite right.

emilio · 2019-11-04T17:06:18Z

@@ -14,16 +14,12 @@ pub fn gen_partialeq_impl(
 ) -> Option<proc_macro2::TokenStream> {
    let mut tokens = vec![];


You should just debug_assert!(!item.is_opaque(..)) if we decide not to derive partialeq for these, then remove the conditional below.

emilio · 2019-11-04T17:07:00Z

 impl CanDeriveHash for Item {
    fn can_derive_hash(&self, ctx: &BindgenContext) -> bool {
-        self.id().can_derive_hash(ctx)
+        self.id().can_derive_hash(ctx) && !self.is_opaque(ctx, &())


Instead of this, the right place for this is returning CanDerive::No here:

rust-bindgen/src/ir/analysis/derive.rs

Line 158 in ec85170

if item.is_opaque(self.ctx, &()) {

Yeah. I started there an then thought that maybe I should "attack" this at the source to make sure i'm not missing other places.
I'll change.

One problem is that this also affects Default and Debug
And for some reason also affected stuff the current code missed like

rust-bindgen/tests/expectations/tests/template.rs

Line 366 in f686751

#[derive(Debug, Default, Copy, Clone, Hash, PartialEq, Eq)]

Well, sure that'd affect more traits, you can choose what it affects conditionally adding something like self.derive_trait.can_derive_opaque().

emilio · 2019-11-09T17:45:49Z

Sorry, I hadn't noticed this was updated (GitHub likes not to notify for force-pushes looks like).

It seems like tests/opaque_pointer.rs doesn't build and CI is red because of that, because we still derive copy on the struct around the opaque thing.

elichai · 2019-11-09T17:48:44Z

Yeah, I'm not really sure what's the right way forward or if this PR is even a wanted feature (it is a breaking change).

I'll play with it a bit more to see if I can get the "right way" to still derive Default and Debug

emilio · 2019-11-09T17:56:33Z

Well, we should clearly not be deriving Copy if any of our members can't automatically derive Copy. Other than that, this feature is fine, though it's indeed breaking I doubt people would rely on it (and if they do, manually memcpying the thing they care about is probably a better way forward).

elichai · 2019-11-09T19:22:02Z

I changed the implementation, but I still don't get why it doesn't implement Debug manually (with the #name: { opaque } thing).
I'll continue to debug that later

elichai · 2019-11-09T19:43:25Z

Ops, I should fix both the rustfmt and the big array problem.
still trying to figure out why it's not manually implementing, it seems like for a single ItemId fn constrain(&mut self, id: ItemId) -> ConstrainResult is called 6-7 times, ~~so it starts with the correct CanDerive::Manually but ends up with either No or Yes somehow.~~

Edit: I just missed the fact that it's a different trait :), it's still running multiple times by for my isolated test it always ends up with Debug -> Manually:

canonical name: OtherOpaque id: ItemId(7), can derive: Debug ? Manually
canonical name: OtherOpaque id: ItemId(12), can derive: Debug ? Manually
canonical name: OtherOpaque id: ItemId(10), can derive: Debug ? Manually
canonical name: OtherOpaque id: ItemId(1), can derive: Debug ? Manually
canonical name: OtherOpaque id: ItemId(12), can derive: Debug ? Manually
canonical name: OtherOpaque id: ItemId(12), can derive: Debug ? Manually
canonical name: OtherOpaque id: ItemId(7), can derive: Debug ? Manually
canonical name: OtherOpaque id: ItemId(10), can derive: Debug ? Manually
canonical name: OtherOpaque id: ItemId(7), can derive: Debug ? Manually
canonical name: OtherOpaque id: ItemId(10), can derive: Debug ? Manually
canonical name: OtherOpaque id: ItemId(1), can derive: Debug ? Manually

So the constain() function isn't the problem.

The problem seem to be ctx.options().impl_debug == false

elichai · 2019-11-09T20:01:42Z

Found the problem :)
So, by default impl_debug: false and derive_debug: true. so by me returning CanDerive::Manually I remove the debug unless the user explicitly asked for it to be implemented.

@emilio what do you think I should do? do you even agree that it makes more sense to print something like OpaqueType { opaque } then OpaqueType{ val: [0,0,0,0] }

emilio · 2019-12-11T02:15:54Z

@emilio what do you think I should do? do you even agree that it makes more sense to print something like OpaqueType { opaque } then OpaqueType{ val: [0,0,0,0] }

I'd be ok with that I think... Either way, really.

elichai · 2020-01-23T11:41:36Z

Hi, sorry I left this as is, context switching is hard.
I'm not sure what to do with the whole union/default thing, do we want it to manually impl default? do we don't? (deriving obviously doesn't work, and that's what is failing this PR right now)

I think this comment tries to explain that but I don't quite understand: https://github.com/rust-lang/rust-bindgen/pull/1667/files#diff-ab2ef507baf5b00651f924fb3cb4241cR51

emilio · 2020-01-26T17:02:25Z

So regarding unions, I think emitting an opaque struct for opaque unions is better, and should save a bit of code.

But I still need to take a closer look here.

bors-servo · 2021-02-18T16:25:22Z

☔ The latest upstream changes (presumably 0e25962) made this pull request unmergeable. Please resolve the merge conflicts.

highfive added the S-awaiting-review label Nov 4, 2019

emilio requested changes Nov 4, 2019

View reviewed changes

elichai force-pushed the 2019-11-opaque branch from 242bbcd to 5d4e52d Compare November 4, 2019 18:13

elichai force-pushed the 2019-11-opaque branch from 5d4e52d to e63dbd8 Compare November 9, 2019 19:21

elichai added 2 commits November 9, 2019 22:13

Disallow Copy/Clone/Eq*/Ord* for opaque types

f705ccf

Update tests without Copy/Clone/Eq*/Ord* derives for opaque types

186501b

elichai force-pushed the 2019-11-opaque branch from e63dbd8 to 186501b Compare November 9, 2019 20:13

highfive added the S-needs-rebase label Feb 18, 2021

pvdrz deleted the branch rust-lang:master November 2, 2023 17:50

pvdrz closed this Nov 2, 2023

		@@ -14,16 +14,12 @@ pub fn gen_partialeq_impl(
		) -> Option<proc_macro2::TokenStream> {
		let mut tokens = vec![];

Conversation

elichai commented Nov 4, 2019

Uh oh!

emilio left a comment

Choose a reason for hiding this comment

Uh oh!

emilio Nov 4, 2019

Choose a reason for hiding this comment

Uh oh!

emilio Nov 4, 2019

Choose a reason for hiding this comment

Uh oh!

elichai Nov 4, 2019

Choose a reason for hiding this comment

Uh oh!

elichai Nov 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emilio Nov 5, 2019

Choose a reason for hiding this comment

Uh oh!

emilio commented Nov 9, 2019

Uh oh!

elichai commented Nov 9, 2019

Uh oh!

emilio commented Nov 9, 2019

Uh oh!

elichai commented Nov 9, 2019

Uh oh!

elichai commented Nov 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elichai commented Nov 9, 2019

Uh oh!

emilio commented Dec 11, 2019

Uh oh!

elichai commented Jan 23, 2020

Uh oh!

emilio commented Jan 26, 2020

Uh oh!

bors-servo commented Feb 18, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

elichai Nov 4, 2019 •

edited

Loading

elichai commented Nov 9, 2019 •

edited

Loading